Skip to content

fix: Dynamically compute default zoom/center for map traces#7884

Open
camdecoster wants to merge 12 commits into
v4.0from
cam/7674/compute-map-default-bounds
Open

fix: Dynamically compute default zoom/center for map traces#7884
camdecoster wants to merge 12 commits into
v4.0from
cam/7674/compute-map-default-bounds

Conversation

@camdecoster

@camdecoster camdecoster commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Description

Use lat/lon points to dynamically compute the default zoom/center for scattermap and densitymap traces.

Closes #7674.

Changes

  • Add check during handleDefaults to dynamically computer map bounds
  • Add tests
  • Linting/formatting

Screenshots

Before After
image image

Testing

  • Be on master
  • Run the following mock through Plotly devtools
    {
      "data": [
        {
          "hovertext": ["San Marino", "Cairo", "Istanbul", "Trondheim"],
          "lat": [43.9360958, 30.06263, 41.01384, 63.43049],
          "lon": [12.4417702, 31.24967, 28.94966, 10.39506],
          "marker": { "color": "#f00" },
          "mode": "markers",
          "type": "scattermap"
        }
      ],
      "layout": { "width": 900, "height": 600 }
    }
  • Note that the Trondheim point is outside of the frame
  • Switch to this branch
  • Run the same mock
  • Note that the scatter points are nicely framed
  • Pan/zoom around
  • Click the reset view button
  • Note that the view is still nicely framed

Notes

@camdecoster camdecoster marked this pull request as ready for review July 1, 2026 23:16
@camdecoster camdecoster added this to the v4.0.0 milestone Jul 1, 2026
Comment thread src/plots/map/layout_defaults.js
});

// Auto-frame the initial view to the data
if (containerIn.center === undefined && containerIn.zoom === undefined) {

@emilykl emilykl Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should skip this step if bounds is defined.

Also, I'm not positive, but I think we should test containerOut rather than containerIn, so that if the user supplies an invalid center or zoom value, we ignore it and compute the auto-bounds anyway.

Edit: Never mind, I misunderstood the bounds param. I do think we should test on containerOut though.

Suggested change
if (containerIn.center === undefined && containerIn.zoom === undefined) {
if (containerOut.center === undefined && containerOut.zoom === undefined) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we test containerOut, center/zoom will always be set because coerce supplies default values for those attributes. If containerIn doesn't have center/zoom, that's a signal that we need to provide bounds for the map. If the user supplies junk, this will still work because the call to getMapFitBounds won't run and coerce already took care of the junk values.

@emilykl emilykl Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah of course we can't use containerOut, that makes sense.

But that does mean auto-bounds won't be applied if the user supplies junk values for center and zoom, right?

(That seems like the wrong behavior to me, but I could be convinced otherwise.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm deferring to the user in this case. If they explicitly set values that would otherwise be set by auto-fitting, I opted to skip auto-fitting. If you want to provide garbage values, that's up to you. We could update that behavior in the future, but I think we should trust the user for now.

Comment thread src/plots/map/map.js
Comment thread src/plots/map/map.js Outdated
Comment on lines +177 to +178
opts._input.center = opts.center = center;
opts._input.zoom = opts.zoom = zoom;

@emilykl emilykl Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change the center and zoom values stored in fullLayout.map, right? I'm not totally sure it's OK to modify fullLayout at this point.

@DhruvGarg111 DhruvGarg111 Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modifying fullLayout at this point , This is actually the same pattern used a few lines below in the moveend
handler, where user pan/zoom writes back to opts._input.center , opts._input.zoom , etc. We need to do it here because MapLibre resolves the bounding box into a concrete center/zoom internally , if we don't capture those values, subsequent updateLayout calls would reset the view to the schema defaults (center 0,0 / zoom 1) instead of the auto-framed position.

please correct me if i am wrong.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay because that's the pattern we're already using in moveend down below. Since we're already mutating these values during pan/zoom, this seemed like an acceptable way to save state.

Comment thread src/plots/map/map.js Outdated
Comment on lines +179 to +184
self.viewInitial = {
center: Lib.extendFlat({}, center),
bearing: opts.bearing,
pitch: opts.pitch,
zoom
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this, since in theory viewInitial is already being set in the plot() function and this feels like an end run around that logic and ripe for bugs. But I'm open to arguments about why this is the best/only way to handle viewInitial in the auto-bounds case.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is timing: viewInitial normally gets set in
plot() before the map's load event fires, at which point opts.center and opts.zoom still hold the schema
defaults since MapLibre hasn't resolved the bounds yet. Without this override, double-clicking "reset view" would
snap to center=(0,0), zoom=1 instead of the auto-framed view the user actually saw. Moving the viewInitial
assignment into the load callback for all cases would be cleaner but felt like too big a refactor for this PR -
happy to hear if you see a better approach though!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update is necessary to make the Reset modebar button and map double click callbacks use the correct values. This seemed like a reasonable place to update the data, but let me explore reworking the plot function.

@camdecoster camdecoster requested a review from emilykl July 8, 2026 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants